Conversation
WyriHaximus
force-pushed
the
3.x-raise-minimum-php-version-to-7.1PLUS
branch
from
1043242 to
92d95c7
Compare
SimonFrings
left a comment
SimonFrings
left a comment
There was a problem hiding this comment.
Thanks for working on this @WyriHaximus, I added a few remarks down below and I also went through the project and noticed a few things we should add to this PR. Here are my findings and thoughts:
-
First of all, I think my suggested changes will make this PR a lot bigger, so maybe it's a good idea to split the last commit into two. This way the PR is a bit more overseeable. Could be something like:
- Commit 2: "Update PHP language syntax and remove legacy workarounds" (including all the array stuff)
- Commit 3: "Update test suite and remove legacy PHPUnit workarounds" (PHPUnit changes + legacy loop extensions in tests)
-
We have a lot of workaround functions inside the
TestCase.phpthat were necessary for PHPUnit < 7.5. Tried this out and I think we can get rid of all of them (regarding thesetExpectedException(),assertContainsString(),assertContainsStringIgnoringCase()andassertSameIgnoringCase()) -
There are 3 places in our test suite where we skip tests for HHVM, we should also remove them (reagarding the
DuplexResourceStreamTest,WritableResourceStreamTestandReadableResourceStreamTest) -
Inside the
DuplexResourceStreamandReadableResourceStremwe have legacy PHP 5 workarounds at the bottom which can be removed in my opinion. -
In line 90 inside the
FunctionalInternetTestclass is a comment describing a bug in PHP < 7.1.4 and PHP < 7.0.18. The part for 7.1.4 should stay, but I think we can remove the part for PHP < 7.0.18.
That's it for now 👍
WyriHaximus
force-pushed
the
3.x-raise-minimum-php-version-to-7.1PLUS
branch
2 times, most recently
from
e4313ea to
1f68392
Compare
WyriHaximus
force-pushed
the
3.x-raise-minimum-php-version-to-7.1PLUS
branch
from
f699f29 to
8bade19
Compare
|
Thanks for the pointers @SimonFrings, updated the PR according to them. |
WyriHaximus
force-pushed
the
3.x-raise-minimum-php-version-to-7.1PLUS
branch
2 times, most recently
from
85b4fe8 to
d23f22a
Compare
SimonFrings
left a comment
SimonFrings
left a comment
There was a problem hiding this comment.
Went through everything, looks good to me. Also nice that you caught a few additional things that I didn't even mention 👍
WyriHaximus
force-pushed
the
3.x-raise-minimum-php-version-to-7.1PLUS
branch
from
65f7f6a to
19a7e3d
Compare
|
Update one small nitpick, and had to bump the macos job a few times until it passed |
WyriHaximus
force-pushed
the
3.x-raise-minimum-php-version-to-7.1PLUS
branch
3 times, most recently
from
5f6dfd1 to
2866134
Compare
clue
left a comment
clue
left a comment
There was a problem hiding this comment.
@WyriHaximus Thank you for looking into this and for your patience! I've added some minor remarks only, otherwise love the direction!
clue
force-pushed
the
3.x-raise-minimum-php-version-to-7.1PLUS
branch
from
1d047d5 to
2f70d54
Compare
clue
left a comment
clue
left a comment
There was a problem hiding this comment.
@WyriHaximus Went over your changes together with @SimonFrings and addressed all outstanding issues, great work, now let's get this shipped! 
For reference: Link to diff between original 1d047d5 and new 2f70d54: https://gist.github.com/clue/7e6012fecc995dee201eb3aae600bcfb)
clue
force-pushed
the
3.x-raise-minimum-php-version-to-7.1PLUS
branch
from
2f70d54 to
1bb9595
Compare
SimonFrings
left a comment
SimonFrings
left a comment
There was a problem hiding this comment.
Went over all changes made, looks good to me so let's get this in 👍
3 participants
This changeset updates the project to require PHP 7.1+ and drop legacy PHP < 7.1 and HHVM as discussed in #173. I'm marking this as a BC break for anybody still stuck on very old PHP versions, but there's little chance this will affect any current projects otherwise.
This PR aims to contain the minimal changeset to update the PHP version requirement only. Follow-up PRs will update our APIs to leverage newer language features.
Builds on top of #172, #174 and others
Refs reactphp/cache#58